Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Setup dependencies that match the GCC ABI of the compiler #604

Merged
merged 8 commits into from
Jan 18, 2020
Merged

Setup dependencies that match the GCC ABI of the compiler #604

merged 8 commits into from
Jan 18, 2020

Conversation

giordano
Copy link
Member

@giordano giordano commented Jan 11, 2020 •
edited
Loading

This should solve the issue we had in JuliaPackaging/Yggdrasil#369 where the build environment had libgfortran3, but a dependency (namely OpenBLAS) was downloaded in its libgfortran5 flavour.

I tested this PR locally with JuliaPackaging/Yggdrasil#378.

@giordano giordano mentioned this pull request Jan 11, 2020
Merged
giordano
giordano commented Jan 11, 2020
View reviewed changes
src/Rootfs.jl Show resolved Hide resolved
This was referenced Jan 14, 2020
Merged
Merged
@giordano giordano mentioned this pull request Jan 16, 2020
Closed
@jd-lara jd-lara mentioned this pull request Jan 16, 2020
Closed
staticfloat
staticfloat requested changes Jan 18, 2020
View reviewed changes
src/DockerRunner.jl
shards = choose_shards(platform; extract_kwargs(kwargs, (:preferred_gcc_version,:preferred_llvm_version,:bootstrap_list,:compilers))...)
if isnothing(shards)
# Choose the shards we're going to mount
shards = choose_shards(platform; extract_kwargs(kwargs, (:preferred_gcc_version,:preferred_llvm_version,:bootstrap_list,:compilers))...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this into the kwarg initialization above? I'd rather avoid the "default value of nothing means initialize it halfway through the method" pattern if we can, and I don't think there's a hidden reason why choose_shards() must be run after the stuff above it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't know kwargs when we initialise shards, do we?

src/UserNSRunner.jl
if isnothing(shards)
# Choose the shards we're going to mount
shards = choose_shards(platform; extract_kwargs(kwargs, (:preferred_gcc_version,:preferred_llvm_version,:bootstrap_list,:compilers))...)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here; let's try moving this initialization into the kwargs specification above.

src/Rootfs.jl Show resolved Hide resolved
src/Rootfs.jl
LLVMBuild(v"7.1.0"),
LLVMBuild(v"8.0.1"),
LLVMBuild(v"9.0.1")]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see us eventually adding Rust, Go, etc... things here. Nice.

src/Rootfs.jl Show resolved Hide resolved
test/basic.jl Outdated Show resolved Hide resolved
test/wizard.jl Outdated Show resolved Hide resolved
@giordano giordano mentioned this pull request Jan 18, 2020
Merged
@giordano
Copy link
Member Author

One thing come out in JuliaPackaging/Yggdrasil#293 that is slightly relevant for the issue this PR is meant to solve. In

BinaryBuilder.jl/src/Runner.jl

Lines 311 to 312 in f4ba291

## Generate compiler wrappers for both our target and our host
for p in unique((platform, host_platform))
we generate the compiler wrappers for (first) the target and (second) the host. The unique makes sure that we don't write things twice, but this actually happens: host_platform is usually Linux(:x86_64, libc=:musl), but if the target has a non-nothing compiler ABI specification then the wrappers are written twice, with the ones for the host (always ABI-agonistic) overwriting the one for the target. In the particular case the non-nothing ABI is the C++ string one, this means that the wrappers with the setting of _GLIBCXX_USE_CXX11_ABI are lost.

Possible solutions I came up with:

  • just invert the order of the loop: write first the wrappers for the host and then those for the target
  • force the host to have the same C++ string ABI as the target, when the host has nothing and the target has non-nothing. When they're both nothing that's fine anyway, when they're both non-nothing and different.... that's a problem that we really can't solve. The ABI coercion can happen in generate_compiler_wrappers!. I was initially thinking about doing this in autobuild, but propagating host_platform requires a little bit more work to fully propagate the platform value.

What do you think?

@giordano
Copy link
Member Author

I went ahead and implemented a mixture of my proposed solutions: let the target wrappers override the host wrappers, but also error out in case we're asking for incompatible ABI. The error situations should never happen in practice as the host_platform keyword argument to generate_compiler_wrappers! is never set from the callers.

@giordano giordano force-pushed the dependencies-abi branch 4 times, most recently from 033bd85 to f6bcbb4 Compare January 18, 2020 16:07
@staticfloat
Copy link
Member

Yes, I agree that this is the best solution.

@staticfloat staticfloat merged commit c12e784 into JuliaPackaging:master Jan 18, 2020
@giordano giordano deleted the dependencies-abi branch January 19, 2020 05:19
This was referenced Jan 19, 2020
Closed
Merged
@pabloferz pabloferz mentioned this pull request Mar 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staticfloat staticfloat staticfloat requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@giordano @staticfloat